Skip to content

Rectify Bluefin NAND on-die ECC support (6.12)#229

Merged
gratian merged 4 commits intoni:nilrt/master/6.12from
HatsyRei:nilrt/master/6.12
Apr 24, 2025
Merged

Rectify Bluefin NAND on-die ECC support (6.12)#229
gratian merged 4 commits intoni:nilrt/master/6.12from
HatsyRei:nilrt/master/6.12

Conversation

@HatsyRei
Copy link
Copy Markdown
Member

@HatsyRei HatsyRei commented Apr 23, 2025 •
edited
Loading

#226 but for nilrt/master/6.12.

WI: AB#3076957

Bluefin utilizes the Micron MT29F2G08ABBEAH4 NAND flash via the onboard PL353 NAND controller. In 4.1, on-die ECC operations along with raw NAND read/write hooks were implemented in the pl353_nand driver. Following 10d4e75, Micron NAND specific logic has been moved into nand_micron.c.

This PR rectifies previous on-die ECC and NAND subpage read/write implementations by implementing NAND on-die ECC subpage read/write support in nand_micron.c instead of pl35x-nand-controller.c.

Testing: Deployed changes onto cDAQ-9189. Previously observed bitflips no longer reproducible after on-die ECC is enabled correctly.

@HatsyRei HatsyRei force-pushed the nilrt/master/6.12 branch 2 times, most recently from 7b4c235 to 445cbb4 Compare April 23, 2025 03:58
@HatsyRei
Copy link
Copy Markdown
Member Author

HatsyRei commented Apr 23, 2025

@chaitu236 I'm not sure why, but checkpatch seems to think bc5d815 is unknown?

WARNING: Unknown commit id 'bc5d81525799d4ff42b5f40b89368778a1a648c8', maybe rebased or not pulled?
10:
(cherry picked from commit bc5d815)

@gratian gratian requested a review from a team April 23, 2025 14:08
@chaitu236
Copy link
Copy Markdown

chaitu236 commented Apr 23, 2025

@chaitu236 I'm not sure why, but checkpatch seems to think bc5d815 is unknown?

WARNING: Unknown commit id 'bc5d81525799d4ff42b5f40b89368778a1a648c8', maybe rebased or not pulled?
10:
(cherry picked from commit bc5d815)

When I run git format-patch nilrt/master/6.12 and ./scripts/checkpatch.pl *patch on my dev machine, I see a somewhat different error (the difference could be due to different git versions being used on my dev machine vs build agent).

----------------------------------------------------
0004-mtd-nand_micron-Enable-subpage-operations.patch
----------------------------------------------------
ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit bc5d81525799 ("mtd: nand_micron: Enable subpage operations")'
#10:
(cherry picked from commit bc5d81525799d4ff42b5f40b89368778a1a648c8)

total: 1 errors, 0 warnings, 70 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

0004-mtd-nand_micron-Enable-subpage-operations.patch has style problems, please review.

In any case, the warning/error seems to be due to incorrect cherry-picking.

You have

mtd: nand_micron: Enable subpage operations

    Existing devices which utilize Micron NAND may contain partitions
    formatted with subpages. Support this use case by implementing hooks
    for subpage read and write in Micron NAND driver.

    (cherry picked from commit bc5d81525799d4ff42b5f40b89368778a1a648c8)
    Signed-off-by: HatsyRei <yan.huan.chng@emerson.com>

Where as it should be something like

    mtd: nand_micron: Enable subpage operations

    Existing devices which utilize Micron NAND may contain partitions
    formatted with subpages. Support this use case by implementing hooks
    for subpage read and write in Micron NAND driver.

    Signed-off-by: HatsyRei <yan.huan.chng@emerson.com>
    (cherry picked from commit bc5d81525799d4ff42b5f40b89368778a1a648c8)
    Signed-off-by: Chaitanya Vadrevu <chaitanya.vadrevu@emerson.com>

Notice the missing signed-off-by before cherry-pick line.

You can fix this by removing the commit and cherry-picking with git cherry-pick -sx bc5d81525799d4ff42b5f40b89368778a1a648c8.

chaitu236
chaitu236 requested changes Apr 23, 2025
View reviewed changes
Copy link
Copy Markdown

@chaitu236 chaitu236 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HatsyRei Your other commit also has this issue btw.

HatsyRei and others added 4 commits April 24, 2025 10:29
@HatsyRei
Revert "ARM: pl35x-nand-controller: Fix subpage read performance"
5f82a63 
This reverts commit b9fd08c.

Signed-off-by: HatsyRei <yan.huan.chng@emerson.com>
@HatsyRei
Revert "pl35x-nand-controller: Enable on-die ECC subpage operations"
40eed26 
This reverts commit 3b79117.

Signed-off-by: HatsyRei <yan.huan.chng@emerson.com>
@harisokanovic @HatsyRei
pl353_nand: Add module params to disable subpage read and write
ecd37c4 
Add `enable_subpage_read` and `enable_subpage_write` load-time options
to toggle subpage read/write operations on pl353 chips. Both options are
enabled by default, and may be toggled in the boot loader.

Signed-off-by: Haris Okanovic <haris.okanovic@ni.com>
Acked-by: Gratian Crisan <gratian.crisan@ni.com>
Acked-by: Jeff Westfahl <jeff.westfahl@ni.com>

Natinst-CAR-ID: 599280
Natinst-ReviewBoard-ID: 151758
(cherry picked from commit 311a5711cfdbfa20e3f4ec2dbd429f96b9f76c2f)
(cherry picked from commit f261b4b)
Signed-off-by: HatsyRei <yan.huan.chng@emerson.com>
(cherry picked from commit 118d09c)
Signed-off-by: HatsyRei <yan.huan.chng@emerson.com>
@HatsyRei
mtd: nand_micron: Enable subpage operations
220bad6 
Existing devices which utilize Micron NAND may contain partitions
formatted with subpages. Support this use case by implementing hooks
for subpage read and write in Micron NAND driver.

Signed-off-by: HatsyRei <yan.huan.chng@emerson.com>
(cherry picked from commit bc5d815)
Signed-off-by: HatsyRei <yan.huan.chng@emerson.com>
@HatsyRei HatsyRei force-pushed the nilrt/master/6.12 branch from 445cbb4 to 220bad6 Compare April 24, 2025 02:30
@HatsyRei
Copy link
Copy Markdown
Member Author

HatsyRei commented Apr 24, 2025

@chaitu236 I redid the cherry picks and got this:

WARNING: Duplicate signature
21:
Signed-off-by: HatsyRei yan.huan.chng@emerson.com

That's actually why I removed the signature from the commit messages the first time around. Strange, are we not supposed to cherry pick our own signed commits?

@chaitu236
Copy link
Copy Markdown

chaitu236 commented Apr 24, 2025

@chaitu236 I redid the cherry picks and got this:

WARNING: Duplicate signature
21:
Signed-off-by: HatsyRei yan.huan.chng@emerson.com

That's actually why I removed the signature from the commit messages the first time around. Strange, are we not supposed to cherry pick our own signed commits?

Interesting. It appears that checkpatch's expectation when cherry-picking self-commits is that there wouldn't be a signed-off-by tag after the "(cherry picked from commit ..." line; removing the last signature gets it to stop complaining.

But whatever checkpatch says, I prefer having commit end with a signed-off-by tag.

@gratian gratian merged commit a8b4e94 into ni:nilrt/master/6.12 Apr 24, 2025
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gratian gratian gratian approved these changes

@chaitu236 chaitu236 chaitu236 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@HatsyRei @chaitu236 @gratian @harisokanovic